Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event Loop & Socket Type Multi-Support #692

Open
wants to merge 71 commits into
base: main
Choose a base branch
from

Conversation

xiazhvera
Copy link
Contributor

@xiazhvera xiazhvera commented Nov 7, 2024

Issue #, if available:

Description of changes:

  • Allow select from different event loop type
  • Allow select from different socket implementation
  • The CI Job was renamed as we introduced new compile options. I will update the required CI once before merge.
    macos -> macos(-DAWS_USE_APPLE_NETWORK_FRAMEWORK=OFF)
    macos-debug -> macos-debug (-DAWS_USE_APPLE_NETWORK_FRAMEWORK=OFF)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 66.02564% with 53 lines in your changes missing coverage. Please review.

Project coverage is 79.66%. Comparing base (b1d0202) to head (e1d7513).

Files with missing lines Patch % Lines
source/event_loop.c 43.10% 33 Missing ⚠️
source/socket.c 74.68% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
- Coverage   80.10%   79.66%   -0.45%     
==========================================
  Files          29       30       +1     
  Lines        6001     6121     +120     
==========================================
+ Hits         4807     4876      +69     
- Misses       1194     1245      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xiazhvera xiazhvera changed the title Event Loop Type Multi-Support Event Loop Type & Socket Multi-Support Nov 8, 2024
@xiazhvera xiazhvera marked this pull request as ready for review November 12, 2024 23:59
include/aws/io/event_loop.h Show resolved Hide resolved
include/aws/io/private/event_loop_impl.h Show resolved Hide resolved
include/aws/io/socket.h Show resolved Hide resolved
include/aws/io/socket.h Outdated Show resolved Hide resolved
@@ -131,7 +131,8 @@ struct aws_event_loop_vtable s_kqueue_vtable = {
.is_on_callers_thread = s_is_event_thread,
};

struct aws_event_loop *aws_event_loop_new_default_with_options(
#ifdef AWS_ENABLE_KQUEUE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we be here if this wasn't defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not defined AWS_ENABLE_KQUEUE, the following function should be defined. (in source/event_loop.c)

#ifndef AWS_ENABLE_KQUEUE
struct aws_event_loop *aws_event_loop_new_with_kqueue(
    struct aws_allocator *alloc,
    const struct aws_event_loop_options *options) {
    (void)alloc;
    (void)options;
    AWS_ASSERT(0);

    AWS_LOGF_DEBUG(AWS_LS_IO_EVENT_LOOP, "Kqueue is not supported on the platform");
    aws_raise_error(AWS_ERROR_PLATFORM_NOT_SUPPORTED);
    return NULL;
}
#endif // AWS_ENABLE_EPOLL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, a rephrasing: "why is this source file being compiled if AWS_ENABLE_KQUEUE is not defined at compile time?"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a compromise due to a limit on Swift package.
The Swift package cannot differentiate between Apple platforms when including source files, so the KQUEUE source file gets included for all Apple platforms, as it is required for macOS. While it is possible to define different compile flags for different Apple platforms (e.g., enabling AWS_ENABLE_KQUEUE only for macOS and not for iOS).
Eventually, on iOS, we end up in a scenario where the kqueue source file is included, but AWS_ENABLE_KQUEUE is disabled.

source/socket.c Outdated Show resolved Hide resolved
source/socket.c Outdated Show resolved Hide resolved
source/socket.c Outdated Show resolved Hide resolved
tests/event_loop_test.c Show resolved Hide resolved
source/windows/iocp/socket.c Outdated Show resolved Hide resolved
include/aws/io/socket.h Outdated Show resolved Hide resolved
include/aws/io/socket.h Outdated Show resolved Hide resolved
@@ -131,7 +131,8 @@ struct aws_event_loop_vtable s_kqueue_vtable = {
.is_on_callers_thread = s_is_event_thread,
};

struct aws_event_loop *aws_event_loop_new_default_with_options(
#ifdef AWS_ENABLE_KQUEUE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, a rephrasing: "why is this source file being compiled if AWS_ENABLE_KQUEUE is not defined at compile time?"

source/event_loop.c Outdated Show resolved Hide resolved
source/socket.c Show resolved Hide resolved
source/socket.c Show resolved Hide resolved
source/windows/iocp/socket.c Show resolved Hide resolved
@xiazhvera
Copy link
Contributor Author

The CI Job was renamed as we introduced new compile options. I will update the required CI once before merge.
macos -> macos(-DAWS_USE_APPLE_NETWORK_FRAMEWORK=OFF)
macos-debug -> macos-debug (-DAWS_USE_APPLE_NETWORK_FRAMEWORK=OFF)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants